rebase: apply and cleanup autostash when rebase fails to start #1772
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
phillipwood
force-pushed
the
rebase-fix-autostash
branch
from
4e6edbe
to
f5b4608
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Wed, Aug 14, 2024 at 01:22:27PM +0000, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <[email protected]>
>
> If "git rebase" fails to start after stashing the user's uncommitted
> changes then it forgets to restore the stashed changes and remove state
s/remove/& the/
> directory. To make matters worse running "git rebase --abort" to apply
s/worse/&,/
> the stashed changes and cleanup the state directory fails because the
s/cleanup/& of/
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e3a8e74cfc2..ac23c4ddbb0 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -526,6 +526,23 @@ static int rebase_write_basic_state(struct rebase_options *opts)
> return 0;
> }
>
> +static int cleanup_autostash(struct rebase_options *opts)
> +{
> + int ret;
> + struct strbuf dir = STRBUF_INIT;
> + const char *path = state_dir_path("autostash", opts);
> +
> + if (!file_exists(path))
> + return 0;
> + ret = apply_autostash(path);
> + strbuf_addstr(&dir, opts->state_dir);
> + if (remove_dir_recursively(&dir, 0))
> + ret = error_errno(_("could not remove '%s'"), opts->state_dir);
This seems somewhat dangerous to me though. Should we really delete the
autostash directory in case applying the autostash has failed?
> @@ -1851,9 +1871,14 @@ run_rebase:
>
> cleanup:
> strbuf_release(&buf);
> + strbuf_release(&msg);
> strbuf_release(&revisions);
> rebase_options_release(&options);
> free(squash_onto_name);
> free(keep_base_onto_name);
> return !!ret;
> +
> +cleanup_autostash:
> + ret |= !!cleanup_autostash(&options);
> + goto cleanup;
> }
It's somewhat curious of a code flow to jump backwards. It might be
easier to reason about the flow if we kept track of a variable
`autostash_needs_cleanup` that we set such that all jumps can go to the
`cleanup` label instead.
Patrick |
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Patrick
On 14/08/2024 15:36, Patrick Steinhardt wrote:
> On Wed, Aug 14, 2024 at 01:22:27PM +0000, Phillip Wood via GitGitGadget wrote:
>> From: Phillip Wood <[email protected]>
>>
>> If "git rebase" fails to start after stashing the user's uncommitted
>> changes then it forgets to restore the stashed changes and remove state
> > s/remove/& the/
> >> directory. To make matters worse running "git rebase --abort" to apply
> > s/worse/&,/
> >> the stashed changes and cleanup the state directory fails because the
> > s/cleanup/& of/
Thanks for catching those typos
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index e3a8e74cfc2..ac23c4ddbb0 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -526,6 +526,23 @@ static int rebase_write_basic_state(struct rebase_options *opts)
>> return 0;
>> }
>> >> +static int cleanup_autostash(struct rebase_options *opts)
>> +{
>> + int ret;
>> + struct strbuf dir = STRBUF_INIT;
>> + const char *path = state_dir_path("autostash", opts);
>> +
>> + if (!file_exists(path))
>> + return 0;
>> + ret = apply_autostash(path);
>> + strbuf_addstr(&dir, opts->state_dir);
>> + if (remove_dir_recursively(&dir, 0))
>> + ret = error_errno(_("could not remove '%s'"), opts->state_dir);
> > This seems somewhat dangerous to me though. Should we really delete the
> autostash directory in case applying the autostash has failed?
Applying the stash should not fail because the rebase has not started and so HEAD, the index and the worktree are unchanged since the stash was created. If it does fail for some reason then apply_autostash() creates a new entry under refs/stash. We definitely do want to remove the directory otherwise we're left with the inconsistent state we're tying to fix.
>> @@ -1851,9 +1871,14 @@ run_rebase:
>> >> cleanup:
>> strbuf_release(&buf);
>> + strbuf_release(&msg);
>> strbuf_release(&revisions);
>> rebase_options_release(&options);
>> free(squash_onto_name);
>> free(keep_base_onto_name);
>> return !!ret;
>> +
>> +cleanup_autostash:
>> + ret |= !!cleanup_autostash(&options);
>> + goto cleanup;
>> }
> > It's somewhat curious of a code flow to jump backwards. It might be
> easier to reason about the flow if we kept track of a variable
> `autostash_needs_cleanup` that we set such that all jumps can go to the
> `cleanup` label instead.
It is slightly odd, but in the end I decided it was simpler to say "goto cleanup_autostash" than try and keep track of what needs cleaning up when saying "goto cleanup". There are a couple of similar examples in builtin/stash.c:show_stash() and config.c:git_config_set_multivar_in_file_gently()
Thanks for the review
Phillip
> Patrick
> |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <[email protected]> writes:
> Applying the stash should not fail because the rebase has not started
> and so HEAD, the index and the worktree are unchanged since the stash
> was created. If it does fail for some reason then apply_autostash()
> creates a new entry under refs/stash. We definitely do want to remove
> the directory otherwise we're left with the inconsistent state we're
> tying to fix.
If it is not expected to fail 99% of times, it feels more prudent to
abort loudly without making further damage to lose information and
ask the user to check what happened in the working tree, rather than
blindly removing the clue to understand what went wrong. For
example, could the reason why applying the stash failed be because
the user forgot that the working tree was being used for rebasing
and mucked with its contents from say another terminal?
Thanks. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Junio
On 14/08/2024 18:27, Junio C Hamano wrote:
> Phillip Wood <[email protected]> writes:
> >> Applying the stash should not fail because the rebase has not started
>> and so HEAD, the index and the worktree are unchanged since the stash
>> was created. If it does fail for some reason then apply_autostash()
>> creates a new entry under refs/stash. We definitely do want to remove
>> the directory otherwise we're left with the inconsistent state we're
>> tying to fix.
> > If it is not expected to fail 99% of times, it feels more prudent to
> abort loudly without making further damage to lose information and
> ask the user to check what happened in the working tree, rather than
> blindly removing the clue to understand what went wrong. For
> example, could the reason why applying the stash failed be because
> the user forgot that the working tree was being used for rebasing
> and mucked with its contents from say another terminal?
If the working tree has changed then the stash will still apply but possibly with conflicts - the same thing can happen when the branch has been successfully rebased. If there are conflicts then the stash is saved in refs/stash as well. This code is just doing what we do at the end of a successful rebase so I'm don't really understand what the issue is. Looking at finish_rebase() we don't even check the return value of apply_autostash() when applying the stash at the end of a successful rebase.
Best Wishes
Phillip
> Thanks.
> |
If "git rebase" fails to start after stashing the user's uncommitted changes then it forgets to restore the stashed changes and remove the state directory. To make matters worse, running "git rebase --abort" to apply the stashed changes and cleanup the state directory fails because the state directory only contains the "autostash" file and is missing the "head-name" and "onto" files required by read_basic_state(). Fix this by applying the autostash and removing the state directory if the pre-rebase hook or initial checkout fail. This matches what finish_rebase() does at the end of a successful rebase. If the user modifies any files after the autostash is created it is possible there will be conflicts when the autostash is applied. In that case apply_autostash() saves the stash in a new entry under refs/stash and so it is safe to remove the state directory containing the autostash file. New tests are added to check the autostash is applied and the state directory is removed if the rebase fails to start. Checks are also added to some existing tests in order to ensure there is no state directory left behind when a rebase fails to start and no autostash has been created. Reported-by: Brian Lyles <[email protected]> Signed-off-by: Phillip Wood <[email protected]>
phillipwood
force-pushed
the
rebase-fix-autostash
branch
from
f5b4608
to
c76c050
Compare
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@1923e46. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@93d3a0d. |
|
This patch series was integrated into seen via git@753bb38. |
|
This patch series was integrated into seen via git@9f8536d. |
|
There was a status update in the "New Topics" section about the branch "git rebase --autostash" failed to resurrect the autostashed changes when the command gets aborted after giving back control asking for hlep in conflict resolution. Will merge to 'next'? source: <[email protected]> |
|
This patch series was integrated into seen via git@064d647. |
|
This patch series was integrated into seen via git@c987856. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <[email protected]> writes:
> ... This code is just doing what we do at
> the end of a successful rebase so I'm don't really understand what the
> issue is. Looking at finish_rebase() we don't even check the return
> value of apply_autostash() when applying the stash at the end of a
> successful rebase.
At that point we give control back the user, so if things are left
in conflicted or any other "unexpected" funny state, the user kill
keep the both halves. As long as the user clearly understands why
the working tree is in such a funny state, we should be OK (and I
would imagine that we are giving messages like "applying preexisting
changes stashed away before rebasing" or something).
Thanks.
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <[email protected]> writes:
> ... This code is just doing what we do at
> the end of a successful rebase so I'm don't really understand what the
> issue is. Looking at finish_rebase() we don't even check the return
> value of apply_autostash() when applying the stash at the end of a
> successful rebase.
At that point we give control back the user, so if things are left
in conflicted or any other "unexpected" funny state, the user kill
keep the both halves. As long as the user clearly understands why
the working tree is in such a funny state, we should be OK (and I
would imagine that we are giving messages like "applying preexisting
changes stashed away before rebasing" or something).
Thanks.
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Phillip Wood via GitGitGadget" <[email protected]> writes:
> Thanks to Junio and Patrick for their comments on V1. I've updated the
> commit message to correct the typos found by Patrick and added an
> explanation of why it is safe to remove the state directory.
Any other comments, or are we all happy with this iteration?
Thanks. |
|
There was a status update in the "Cooking" section about the branch "git rebase --autostash" failed to resurrect the autostashed changes when the command gets aborted after giving back control asking for hlep in conflict resolution. Will merge to 'next'? source: <[email protected]> |
|
This patch series was integrated into seen via git@5904059. |
|
There was a status update in the "Cooking" section about the branch "git rebase --autostash" failed to resurrect the autostashed changes when the command gets aborted after giving back control asking for hlep in conflict resolution. Will merge to 'next'? source: <[email protected]> |
|
This patch series was integrated into seen via git@4a5ecc3. |
|
This patch series was integrated into seen via git@712db21. |
|
This patch series was integrated into seen via git@37e4ccb. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Junio C Hamano <[email protected]> writes:
> "Phillip Wood via GitGitGadget" <[email protected]> writes:
>
>> Thanks to Junio and Patrick for their comments on V1. I've updated the
>> commit message to correct the typos found by Patrick and added an
>> explanation of why it is safe to remove the state directory.
>
> Any other comments, or are we all happy with this iteration?
Let me mark the topic for 'next'. Thanks for reporting, fixing and
reviewing. |
|
This patch series was integrated into seen via git@fca532b. |
|
There was a status update in the "Cooking" section about the branch "git rebase --autostash" failed to resurrect the autostashed changes when the command gets aborted after giving back control asking for hlep in conflict resolution. Will merge to 'next'. source: <[email protected]> |
|
This patch series was integrated into seen via git@00c6512. |
|
This patch series was integrated into next via git@6b41d66. |
|
This patch series was integrated into seen via git@2dd42a0. |
|
There was a status update in the "Cooking" section about the branch "git rebase --autostash" failed to resurrect the autostashed changes when the command gets aborted after giving back control asking for hlep in conflict resolution. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@d98ac3d. |
|
There was a status update in the "Cooking" section about the branch "git rebase --autostash" failed to resurrect the autostashed changes when the command gets aborted after giving back control asking for hlep in conflict resolution. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@a091155. |
|
This patch series was integrated into seen via git@bc78ebe. |
|
There was a status update in the "Cooking" section about the branch "git rebase --autostash" failed to resurrect the autostashed changes when the command gets aborted after giving back control asking for hlep in conflict resolution. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@7ff6096. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thanks to Junio and Patrick for their comments on V1. I've updated the commit message to correct the typos found by Patrick and added an explanation of why it is safe to remove the state directory.
In his review Patrick suggested removing the backwards jump after applying the autostash and cleaning up the state directory in favor of a variable that tracks if we need to apply the autostash. I've decided not to do that as I think having to set a variable before jumping to a cleanup label is more complicated and error prone. There are similar backward jumps in builtin/stash.c:show_stash() and config.c:git_config_set_multivar_in_file_gently()
Thanks to Brian for reporting this. This patch is based on maint, when merging with master there is an easily resolved conflict with a change from 'ps/config-wo-the-repository' which modifies an adjacent line.
Cc: Brian Lyles [email protected]
cc: Patrick Steinhardt [email protected]
cc: Phillip Wood [email protected]